Skip to content

Fix and simplify ALB/ELB protocol option logic#413

Merged
UserNotFound merged 3 commits intomasterfrom
fix-protocols-options
May 1, 2026
Merged

Fix and simplify ALB/ELB protocol option logic#413
UserNotFound merged 3 commits intomasterfrom
fix-protocols-options

Conversation

@UserNotFound
Copy link
Copy Markdown
Member

@UserNotFound UserNotFound commented Apr 30, 2026

There was conflicting logic for which option set was used when a vhost was both ALB and TLS. HTTP endpoints properly list the PFS options now.

[aptible-cli]$ bundle exec bin/aptible help endpoints:https:create 2>&1 | grep protocols
      [--ssl-protocols-override=SSL_PROTOCOLS_OVERRIDE]                # Specify the allowed SSL protocols. Valid options: "TLSv1 TLSv1.1 TLSv1.2", "TLSv1 TLSv1.1 TLSv1.2 PFS", "TLSv1.1 TLSv1.2", "TLSv1.1 TLSv1.2 PFS", "TLSv1.2", "TLSv1.2 PFS", "TLSv1.2 PFS TLSv1.3", "TLSv1.3". PFS options require an HTTPS (ALB) endpoint. Use "default" to reset to the platform default
[aptible-cli]$ bundle exec bin/aptible help endpoints:https:modify 2>&1 | grep protocols
      [--ssl-protocols-override=SSL_PROTOCOLS_OVERRIDE]                # Specify the allowed SSL protocols. Valid options: "TLSv1 TLSv1.1 TLSv1.2", "TLSv1 TLSv1.1 TLSv1.2 PFS", "TLSv1.1 TLSv1.2", "TLSv1.1 TLSv1.2 PFS", "TLSv1.2", "TLSv1.2 PFS", "TLSv1.2 PFS TLSv1.3", "TLSv1.3". PFS options require an HTTPS (ALB) endpoint. Use "default" to reset to the platform default
[aptible-cli]$ bundle exec bin/aptible help endpoints:tls:create 2>&1 | grep protocols
      [--ssl-protocols-override=SSL_PROTOCOLS_OVERRIDE]                  # Specify the allowed SSL protocols. Valid options: "TLSv1 TLSv1.1 TLSv1.2", "TLSv1.1 TLSv1.2", "TLSv1.2", "TLSv1.3". Use "default" to reset to the platform default
[aptible-cli]$ bundle exec bin/aptible help endpoints:tls:modify 2>&1 | grep protocols
      [--ssl-protocols-override=SSL_PROTOCOLS_OVERRIDE]                  # Specify the allowed SSL protocols. Valid options: "TLSv1 TLSv1.1 TLSv1.2", "TLSv1.1 TLSv1.2", "TLSv1.2", "TLSv1.3". Use "default" to reset to the platform default
[aptible-cli]$ bundle exec bin/aptible help endpoints:grpc:create 2>&1 | grep protocols
      [--ssl-protocols-override=SSL_PROTOCOLS_OVERRIDE]                  # Specify the allowed SSL protocols. Valid options: "TLSv1 TLSv1.1 TLSv1.2", "TLSv1.1 TLSv1.2", "TLSv1.2", "TLSv1.3". Use "default" to reset to the platform default
[aptible-cli]$ bundle exec bin/aptible help endpoints:grpc:modify 2>&1 | grep protocols
      [--ssl-protocols-override=SSL_PROTOCOLS_OVERRIDE]                  # Specify the allowed SSL protocols. Valid options: "TLSv1 TLSv1.1 TLSv1.2", "TLSv1.1 TLSv1.2", "TLSv1.2", "TLSv1.3". Use "default" to reset to the platform default
[aptible-cli]$ bundle exec bin/aptible help endpoints:tcp:create 2>&1 | grep protocols
[aptible-cli]$ bundle exec bin/aptible help endpoints:tcp:modify 2>&1 | grep protocols
[aptible-cli]$

@UserNotFound UserNotFound requested a review from benjodo April 30, 2026 23:03
benjodo
benjodo previously approved these changes Apr 30, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@benjodo benjodo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alex, I took a look and it looks functionally good to me. The following are findings that gpt 5.5 found, but I'm considering these nits; I'm approving as-is, and you're welcome to address these if you agree with them and send it back to me for re-review if you choose.

Finding (nit): The regression is not covered by tests; current specs bypass the Thor option declaration/help surface where this bug lived.

The branch changes which --ssl-protocols-override description is declared for ALB+TLS endpoints:

option(
  :ssl_protocols_override,
  type: :string,
  desc: SSL_PROTOCOL_ALB_DESC
)
# ...
unless builder.alb?
  option(
    :ssl_protocols_override,
    type: :string,
    desc: SSL_PROTOCOL_ELB_DESC
  )

But the existing endpoint specs only verify runtime handling after options has already been stubbed:

context '--ssl-protocols-override' do
  it 'passes a valid non-PFS value' do
    wanted = { 'SSL_PROTOCOLS_OVERRIDE' => 'TLSv1.2' }
    expect_create_vhost(service, {}, { settings: wanted })
    stub_options(ssl_protocols_override: 'TLSv1.2')
    subject.send(method, 'web')
  end

That means the exact issue described in the PR, conflicting declaration logic for ALB+TLS help/options, can regress without a failing test.

Recommended fix: add focused coverage for the generated option descriptions, either by inspecting Thor’s registered options for endpoints:https:create, endpoints:https:modify, endpoints:tls:*, endpoints:grpc:*, and endpoints:tcp:*, or by adding CLI help-output specs. At minimum, assert HTTPS includes PFS values and TLS/gRPC do not.

Finding (nit): The invalid-protocol error path still prints SSL_PROTOCOL_VALUES instead of the semantic constant used for validation.

unless ALB_PROTOCOL_VALUES.include?(proto)
  raise Thor::Error,
        "Invalid --ssl-protocols-override: \"#{proto}\". " \
        "Valid options are: #{SSL_PROTOCOL_VALUES.join(', ')}"
end

Today ALB_PROTOCOL_VALUES aliases SSL_PROTOCOL_VALUES, so this is not a functional bug, but it weakens the cleanup by keeping one path coupled to the old generic name.

Recommended fix: change the message to use ALB_PROTOCOL_VALUES.join(', '), matching the validation branch.

@UserNotFound
Copy link
Copy Markdown
Member Author

UserNotFound commented May 1, 2026

Thanks, tests are easy to add!

@UserNotFound UserNotFound requested a review from benjodo May 1, 2026 15:10
benjodo
benjodo approved these changes May 1, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@benjodo benjodo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed that findings were addressed.

@UserNotFound UserNotFound merged commit 0425573 into master May 1, 2026
10 checks passed
@promptless
Copy link
Copy Markdown

promptless Bot commented May 1, 2026

Promptless prepared a documentation update related to this change.

Triggered by aptible/aptible-cli PR #413

Documents the corrected SSL protocol options for different endpoint types. PFS options (like TLSv1.2 PFS) are now correctly documented as only available on HTTPS (ALB) endpoints, while TLS and gRPC (ELB) endpoints support only non-PFS protocols.

Review: Document CLI endpoint settings options

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benjodo benjodo benjodo approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@UserNotFound @benjodo